Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ta_set_timeout can fail to set an alarm #2127

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

peterharperuk
Copy link
Contributor

If alarm_pool_irq_handler takes <1us between handling an alarm and calling ta_set_timeout then no alarms will be set as it will appear as if an earlier alarm is already armedi before the target time.

Make sure ta_set_timeout always leaves with an alarm set by checking the armed status.

Fixes #2118

If alarm_pool_irq_handler takes <1us between handling an alarm and
calling ta_set_timeout then no alarms will be set as it will appear as
if an earlier alarm is already armedi before the target time.

Make sure ta_set_timeout always leaves with an alarm set by checking the
armed status.

Fixes raspberrypi#2118
@kilograham
Copy link
Contributor

From #2118:



        if (earliest_target != -1) { // cancelled alarm has target of -1
            ta_set_timeout(timer, timer_alarm_num, earliest_target);
        }
        // check we haven't now passed the target time; if not we don't want to loop again
    } while ((earliest_target - (int64_t)ta_time_us_64(timer)) <= 0);

why does this not loop?

@peterharperuk
Copy link
Contributor Author

why does this not loop?

earliest_target is correct and > the current time so the loop will exit.

We've just called ta_set_timeout with earliest_target, but that's no guarantee that the alarm was set to this time IF it thinks there's an earlier alarm. It will wrongly think there's an earlier alarm set if the current alarm time == current time.

@kilograham
Copy link
Contributor

ok, this is really difficult to follow;

please annotate the callee and caller code (the two fragments above) with all the values at each place

@peterharperuk
Copy link
Contributor Author

ok, this is really difficult to follow;

// Everything happens in alarm_pool_irq_handler
// ta_time_us_64=10000: IRQ goes off for the only alarm
static void alarm_pool_irq_handler(void) {
    <snip>
    int64_t earliest_target;
    do {
        <snip>
        if (earliest_index >= 0) {
            alarm_pool_entry_t *earliest_entry = &pool->entries[earliest_index];
            earliest_target = earliest_entry->target;
// ta_time_us_64=10000, earliest_target=10000: time to call the callback
            if (((int64_t)ta_time_us_64(timer) - earliest_target) >= 0) {
                <snip>
                int64_t delta;
                if (earliest_target >= 0) {
// Now we call the callback which executes quickly and returns the time it wants to be called next, e.g. 20000us
                    if (earliest_entry->callback == repeating_timer_marker) {
                        repeating_timer_t *rpt = (repeating_timer_t *)earliest_entry->user_data;
                        delta = rpt->callback(rpt) ? rpt->delay_us : 0;
                    } else {
                        alarm_id_t id = make_alarm_id(pool->ordered_head, earliest_entry->sequence);
                        delta = earliest_entry->callback(id, earliest_entry->user_data);
                    }
// delta=20000 (or -20000 for a repeating alarm, it doesn't matter)
                    <snip>
                    if (delta) {
                        int64_t next_time;
                        if (delta < 0) {
                            next_time = earliest_target - delta;
                        } else {
                            next_time = (int64_t) ta_time_us_64(timer) + delta;
                        }
// next_time=30000 the time we want the next alarm to go off
                        earliest_entry->target = next_time;
                        <snip>
                }
            }
        }
        <snip>
        earliest_target = earliest_entry->target;
        if (earliest_target != -1) { // cancelled alarm has target of -1
// earliest_target=30000, ta_time_us_64=10000: we've got to this point in <1us
// we call ta_set_timeout asking for an alarm at earliest_target (30000), this is the code expanded...
            uint32_t current = timer_time_us_32(timer_hw_from_timer(timer));
// current=10000: we've got to this point in <1us
            uint32_t time_til_target = (uint32_t) target - current;
// time_til_target=20000: what we asked for - we still haven't taken > 1us
            uint32_t time_til_alarm = timer_hw_from_timer(timer)->alarm[alarm_num] - current;
// time_til_alarm=0: This is the time for the alarm that's just gone off
// time_til_target [20000] < time_til_alarm [0] is FALSE so the alarm target is NOT set
            if (time_til_target < time_til_alarm) {
// We don't get here
                timer_hw_from_timer(timer)->alarm[alarm_num] = (uint32_t) target;
            }
        }
// earliest_target=30000
// ta_time_us_64=10010: Let's pretend that time has advanced a bit, it doesn't matter as earliest_target is still a long way in the future
    } while ((earliest_target - (int64_t)ta_time_us_64(timer)) <= 0);
// The IRQ exits but it hasn't actually set the alarm that was requested
    <snip>
}

@kilograham
Copy link
Contributor

OK, i agree this makes sense....

Also while looking, i think we should clear the arming in ta_disable_irq_handler

can you please add that to this PR

@peterharperuk
Copy link
Contributor Author

Also while looking, i think we should clear the arming in ta_disable_irq_handler

Good spot. I've added that change.

@kilograham kilograham added this to the 2.2.0 milestone Dec 6, 2024
@kilograham kilograham merged commit 969f589 into raspberrypi:develop Dec 6, 2024
4 checks passed
@kilograham
Copy link
Contributor

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants